Log all Fiber errors w/ component stack#8756
Conversation
A new module has been added (ReactFiberErrorLogger). By default this module just logs error information (call stack and component stack) to the console to make errors easier to debug. In the future, perhaps we will enable users to inject their own handler for custom processing/logging.
| function logCapturedError(capturedError : CapturedError) : void { | ||
| if (__DEV__) { | ||
| // console.log rather than console.error to avoid breaking tests | ||
| // (Jest complains about unexpected console.error calls.) |
There was a problem hiding this comment.
Wouldn't it actually be useful to break tests? e.g. product tests.
We can always spyOn(console, 'error') in error boundary tests.
There was a problem hiding this comment.
+1, though we don't fail tests on console.error at FB, just in this repo.
There was a problem hiding this comment.
The problem is that tests (at least ones I spot-checked) are already asserting that errors occurred (eg spying on console.error and asserting a specific number of calls). This increases the number though and so the tests failed.
I wanted to limit the size of this commit. But I'd be happy to update the 20-30 tests if you guys have a strong preference. 😄
Edit: I'll swap the 2 console.log calls with a single console.error call and update the tests accordingly.
There was a problem hiding this comment.
I'd prefer if we update the tests to assert on the logs, thanks for checking.
There was a problem hiding this comment.
This is proving to be a little complicated since Stack doesn't behave the same as Fiber in terms of error-handling behavior. It isn't a matter of just adding console.error calls to Stack.
There was a problem hiding this comment.
I think an appropriate solution for now is to globally mock ReactFiberErrorLogger (so that Stack and Fiber maintain parity regarding console.error calls). This prevents our tests from forking all over the place.
| expect(() => ReactNoop.flush()).toThrow('Error!'); | ||
| }); | ||
|
|
||
| if (ReactDOMFeatureFlags.useFiber) { |
There was a problem hiding this comment.
ReactNoop used in these tests is already Fiber-specific so this shouldn’t be necessary.
|
Can you show a screenshot of the console for this? |
| try { | ||
| logCapturedError(capturedError); | ||
| } catch (e) { | ||
| // Prevent cycle if logCapturedError() throws. |
There was a problem hiding this comment.
Can we console.error once here too?
The left triangle can be clicked to expand the call stack. Suggestions for improved wording welcome! I left a TODO for linking to |
|
The flippy triangle shows you the console.error stack, not the caught error stack right? If so, let's just include the stack as text. For wording, what do you think of: (componentName ? `React caught an error thrown by ${componentName}. ` : `React caught an error thrown by one of your components. `) +
'You should fix this error in your code. ' +
(
isUsingErrorBoundary ?
`This error will be handled by the error boundary ${errorBoundaryName}.` :
(willRetry ? `React will try to recreate this component tree from scratch. ` : `Recreating the tree from scratch failed, so React will unmount the tree. `) +
`Consider adding an error boundary to your tree to customize error handling behavior.`
) |
|
I am worried that the component stack and display names will be useless for in prod mode with minified code. Maybe omit the stack and names in prod? Or I guess we could have some heuristic to find minified identifiers (/[a-z0-9]{1,2}/i maybe) and ignore them. |
This message is currently logged in |
|
I considered checking if they were already using an error boundary (as you mentioned) but didn't. I wasn't sure if I was overly engineering the message. Since you mentioned it too though, I'll add it. |
|
Can we log a stripped-down message in prod too? I'm concerned about the case where people have promises eating their errors and don't even know their components are throwing. |
|
Definitely. |
|
I love the suggested wording improvements! 😁 Here's an example with a component that defines an error boundary but fails on both attempts: And here's one with no error boundary: In production the following is all that we log: |
bvaughn
force-pushed
the
fb-internal-error-dialog
branch
from
c6b46d8 to
88d2d5c
Compare
| describe('ReactFiberErrorLogger', () => { | ||
| function initReactFiberErrorLoggerMock(mock) { | ||
| jest.resetModules(); | ||
| if (mock) { |
There was a problem hiding this comment.
This is awkward. I'm not sure how to test both unmocked and mocked without explicitly doing this.
|
(Back to you @spicyj) |
sophiebits
left a comment
sophiebits
left a comment
There was a problem hiding this comment.
this looks great to me!
| : 'React caught an error thrown by one of your components.'; | ||
|
|
||
| let errorBoundaryMessage; | ||
| if (errorBoundaryFound) { |
There was a problem hiding this comment.
(errorBoundaryFound && errorBoundaryName might be a little cleaner if you like)
4 participants
A new module has been added (
ReactFiberErrorLogger). This logs error information (call stack and component stack) to the console to make errors easier to debug. It also prompts users to use error boundaries if they are not already using them.In the future, perhaps this will be injectable, enabling users to provide their own handler for custom processing/logging. For the time being, this should help with issues like this / #2461.